-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Python: Modernize 4 queries for missing/multiple calls to init/del methods #19932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Python: Modernize 4 queries for missing/multiple calls to init/del methods #19932
Conversation
QHelp previews: python/ql/src/Classes/CallsToInitDel/MissingCallToDel.qhelpMissing call to superclass
|
… change alerts to alert on the class and provide clearer information, using optional location links.
…ke much sense (__init__ is still called anyway)
3a1ccc1
to
e8a65b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overall this looks really solid, but I must admit I didn't have time to fully digest the new implementation. (However, as I'm going away on PTO, I figured it was best to give you my interim review regardless.)
I'm slightly worried about the many not exists(...)
constructions in the modelling (as these may result in large joins). Make sure the performance testing is run with the tuple-counting option enabled.
Other than that, if the performance looks good, I would be happy to merge this.
Function getASuperCallTargetFromClass(Class mroBase, Class cls, string name) { | ||
exists(Function target | | ||
target = findFunctionAccordingToMroKnownStartingClass(cls, mroBase, name) and | ||
( | ||
result = target | ||
or | ||
result = getASuperCallTargetFromCall(mroBase, target, _, name) | ||
) | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MRO business is a bit awkward. I wonder if we could clean it up by creating a new IPA type representing "the MRO starting at a particular base class", and thus avoid having to thread mroBase
through everything (which might mean we could use the built-in transitive closure operators).
This is just some musing on my part -- not necessarily an immediately actionable suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be a useful component of a public-facing dataflow dispatch / call graph resolution API
) | ||
} | ||
|
||
/** Holds if `meth` calls `super().<name>` where `name` is the name of the method. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment needs updating? I don't see name
anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was meant to refer to the name of the method; updated for more clarity
Modernizes py/missing-call-to-init, py/missing-call-to-del, py/multiple-calls-to-init,, and py/multiplecalls-to-del.
Uses DataFlowDispatch methods rather than. pointsTo.
Improves precision (to reduce FPs) in resolving relevant calls as precisely as possible using known MRO information, improves descriptiveness of alert messages by including more possible relevant locations, and improves documentation.